Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate to edition 2021 #967

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Migrate to edition 2021 #967

wants to merge 6 commits into from
+47 −65

Conversation

Philippe-Cholet
Copy link
Member

I noted 5 months ago that edition 2021 requires rustc 1.56+ so it's now possible.
Is there any reason to not update?

@Philippe-Cholet
Migrate to edition 2021
df6a5c9 
Edition 2021 requires rustc 1.56+ so it's possible.
1. `cargo fix --edition`
2. Field "edition" updated in "Cargo.toml".
@codecov Codecov
Copy link

codecov bot commented Jul 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.72%. Comparing base (6814180) to head (8181f3f).
Report is 111 commits behind head on master.

Files Patch % Lines
src/adaptors/mod.rs 91.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #967      +/-   ##
==========================================
+ Coverage   94.38%   94.72%   +0.33%     
==========================================
  Files          48       49       +1     
  Lines        6665     7222     +557     
==========================================
+ Hits         6291     6841     +550     
- Misses        374      381       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Philippe-Cholet Philippe-Cholet marked this pull request as draft July 4, 2024 10:50
@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Jul 4, 2024 •
edited
Loading

I added some marginal changes:

  • MSRV automated in the crate documentation (valid for a MSRV >= 1.63.0).
  • Fix some additional clippy lints, mostly related to the recent MSRV/edition changes.
  • Use ControlFlow?!

@jswrenn
I remember your #780 (comment) where you prefered ControlFlow over Result, I replaced some, left some aside for now.
Can you elaborate on the reason?

@Philippe-Cholet
Automate MSRV change in the crate doc
2711f1d 
I noted some months ago that the environment variable CARGO_PKG_RUST_VERSION was publicized in 1.63.0 and that if we do not go back before 1.63 then we can automate the MSRV there.
Remain "ci.yml" but I don't see a way to automate it, even with `cargo metadata --no-deps --format-version=1 | jq -r '.packages[0].version'`.
@Philippe-Cholet
Use bool::then_some
ca8a8fb
@Philippe-Cholet
Simplifications from clippy
df00c92 
`clippy::uninlined_format_args` and 2 more.
@Philippe-Cholet
ControlFlow instead of Result
c9d0aef
@Philippe-Cholet Philippe-Cholet marked this pull request as ready for review July 4, 2024 13:14
@Philippe-Cholet
Drop use of deprecated form "default_features"
8181f3f 
This commit fixes the following "warning: `default_features` is deprecated in favor of `default-features` and will not work in the 2024 edition (in the `quickcheck` dependency)".
It's quite off-topic but I don't wish to make another PR to change a single character.
@jswrenn
Copy link
Member

jswrenn commented Aug 2, 2024

I remember your #780 (comment) where you prefered ControlFlow over Result, I replaced some, left some aside for now.
Can you elaborate on the reason?

Result::Err denotes "something went wrong"; ControlFlow::Break doesn't. Favor ControlFlow when we're trying to encode conditional iteration without judgement about error-ness, and favor Result when you're specifically dealing with an early return due to error.

jswrenn
jswrenn approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me; I'll let you hit 'Merge' when you're ready to do so.

src/lib.rs
@@ -49,7 +49,9 @@
//!
//! ## Rust Version
//!
//! This version of itertools requires Rust 1.63.0 or later.
//! This version of itertools requires Rust
#![doc = env!("CARGO_PKG_RUST_VERSION")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge fan of this!

@jswrenn
Copy link
Member

jswrenn commented Oct 22, 2024

@Philippe-Cholet is this good to merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jswrenn jswrenn jswrenn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Philippe-Cholet @jswrenn